Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor PropertyValue to TS #4686

Merged
merged 10 commits into from
Jun 11, 2021
Merged

Refactor PropertyValue to TS #4686

merged 10 commits into from
Jun 11, 2021

Conversation

samwinslow
Copy link
Contributor

@samwinslow samwinslow commented Jun 10, 2021

Changes

Goal: Make state management in PropertyValue more clear; make debugging easier via static typing. It should now be evident that PropertyValue handles string values and array values quite differently from each other.

Closes #4665.

UPDATE:

As I want to get this done in a timely manner and not spend any extra time building a controlled dropdown state, for now I updated the copy on user paths to suggest the user has to start typing in order for results to show. (Select --> Search). WIP

A better fix would be somewhat simple; all we need to do is trigger the dropdown to open after results are loaded (requiring controlled state). I just want to get this done for now, though.

Screen Shot 2021-06-10 at 9 11 50 AM

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests
  • Migrations are safe to run at scale (e.g. PostHog Cloud) – present proof if not obvious
  • Frontend/CSS is usable at 320px (iPhone SE) and decent at 360px (most phones)

@timgl timgl temporarily deployed to posthog-pr-4686 June 10, 2021 12:34 Inactive
@timgl timgl temporarily deployed to posthog-pr-4686 June 10, 2021 12:50 Inactive
@samwinslow samwinslow linked an issue Jun 10, 2021 that may be closed by this pull request
2 tasks
@timgl timgl temporarily deployed to posthog-pr-4686 June 10, 2021 13:11 Inactive
@samwinslow samwinslow requested a review from yakkomajuri June 10, 2021 13:15
@yakkomajuri
Copy link
Contributor

Testing this now - my first comment on this is the same as @timgl had on my first "fix", how do I know what to type on the paths input? I should be getting suggestions right away. Is that what you meant by autofocus? It's indeed annoying because onClick will also trigger on the options in the selector

@yakkomajuri
Copy link
Contributor

Another issue is something I was also looking to fix (I think it may have always been broken) - is that changing PATH TYPE keeps the old input but renders the default path for the new type.

@yakkomajuri
Copy link
Contributor

yakkomajuri commented Jun 10, 2021

Also some issues with the suggestions when switching path types (multiple for the same, suggestions persist changing types)

Screenshot 2021-06-10 at 11 15 03

Screenshot 2021-06-10 at 11 15 23

Copy link
Contributor

@yakkomajuri yakkomajuri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll review the code once the behavioral issues above are addressed. Like I said on our call, there's a lot of problems here beyond just types.

I got beat up by this component yesterday 😂

@yakkomajuri
Copy link
Contributor

Another issue here (also related to legacy bad practices), is that the query for events expects an integer, so it's failing now as we're sending it a string. Previously, we would show autocomplete options as strings but then assign their id as the value, so the user would see a random 294 instead of the event selected. It's ridiculous, I know 😂

This is another issue I was facing

@samwinslow
Copy link
Contributor Author

Testing this now - my first comment on this is the same as @timgl had on my first "fix", how do I know what to type on the paths input? I should be getting suggestions right away. Is that what you meant by autofocus? It's indeed annoying because onClick will also trigger on the options in the selector

Argh, thought I submitted the updated description. Yes, that's what I meant by autocomplete -- will work on today.

@samwinslow
Copy link
Contributor Author

samwinslow commented Jun 10, 2021

Why do we separate Path Type and Start Point? Seems weird to make the user explicitly decide between autocaptured and custom events, for example, when those types are not differentiated elsewhere.

I know this might seem like a UX nitpick, but the weird state mismatches that occur when switching from one path type to another might be solved, for instance, by using SelectBox like we do elsewhere.

EDIT: See images and more context here: #4703.

@samwinslow samwinslow force-pushed the fix-user-paths-bug branch from 9927051 to 223b5fd Compare June 10, 2021 15:43
@timgl timgl temporarily deployed to posthog-pr-4686 June 10, 2021 15:43 Inactive
@timgl timgl temporarily deployed to posthog-pr-4686 June 10, 2021 20:44 Inactive
@samwinslow
Copy link
Contributor Author

Spinning off some of these issues: #4703, #4704 which are long-standing problems. If anyone has advice regarding getting the autofocus to work, it would be much appreciated. I feel like we need to keep this PR to a reasonable size.

@samwinslow
Copy link
Contributor Author

@EDsCODE @paolodamico I see you both did some work on PropertyValue some time ago... if you have any time to take a look, could you see if I'm missing something regarding getting the dropdown to open as soon as results are loaded? Have been stuck on this for a while.

@mariusandra
Copy link
Collaborator

mariusandra commented Jun 11, 2021

I gave it a go and ~90min later I think I have something that works.

I think the problem with the dropdown not opening was this:

<AutoComplete>
   {input ? <Option ... /> : null}
   {options.map(o => <Option ... />)}
</AutoComplete>

The first element was null / undefined and hence the list looked empty and antd didn't auto-open it. Replacing this with an array of options (still passed as children since we need custom labels) solved the problem.

There was also a problem with the "custom element" view, where the text <span> bla was sent to the API, not the ID. That's also fixed.

Plus various type fixes and other small tweaks to make sure things work.

The only thing that's still broken is this: If you search for a custom element, we put the ID in the URL. If you then reload the page, you'll only see the ID in the dropdown, as we haven't downloaded the list of elements and can't really match this ID to any better string. Since the custom element paths page itself is quite useless, I propose to leave it at that for now and move on. :)

@mariusandra mariusandra requested a review from yakkomajuri June 11, 2021 09:57
@mariusandra mariusandra temporarily deployed to posthog-pr-4686 June 11, 2021 09:58 Inactive
@mariusandra mariusandra temporarily deployed to posthog-pr-4686 June 11, 2021 10:22 Inactive
@mariusandra
Copy link
Collaborator

Also managed to find and fix a sql injection (cc @EDsCODE)

@samwinslow @paolodamico @yakkomajuri do you think this is safe to merge now? Obviously cypress is borked and we don't know if anything is broken or not 😅

@mariusandra mariusandra temporarily deployed to posthog-pr-4686 June 11, 2021 10:29 Inactive
@paolodamico
Copy link
Contributor

I actually have little context on this, so I'll defer to @samwinslow, though do let me know if you want me to take a closer look.

@yakkomajuri
Copy link
Contributor

@mariusandra there's still the problem of switching path types with the input persisting but the graph changing.

So it says hogflix.com but it actually is <span> Click me

@mariusandra mariusandra temporarily deployed to posthog-pr-4686 June 11, 2021 12:57 Inactive
@mariusandra
Copy link
Collaborator

That should also be fixed now. Hopefully without breaking anything else :D

@timgl timgl temporarily deployed to posthog-pr-4686 June 11, 2021 13:05 Inactive
@timgl timgl temporarily deployed to posthog-pr-4686 June 11, 2021 14:05 Inactive
Copy link
Contributor

@yakkomajuri yakkomajuri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behavior-wise :thisisfine: - will leave the code review of @mariusandra's changes to @samwinslow

@samwinslow
Copy link
Contributor Author

Re-running all tests locally to see what failures, if any, should be addressed.

@samwinslow samwinslow merged commit 399bcc6 into master Jun 11, 2021
@samwinslow samwinslow deleted the fix-user-paths-bug branch June 11, 2021 18:21
This was referenced Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding a filter in Sessions page crashes
5 participants